-
Notifications
You must be signed in to change notification settings - Fork 224
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: Drop dependency on fact_helper_file #1140
base: master
Are you sure you want to change the base?
Conversation
402d528
to
ea0845f
Compare
While I don't really care note that the repo is in my namespace and not fkiecad's. |
ea0845f
to
855101f
Compare
This is blocked on ahupp/python-magic#304 which allows us to delete |
855101f
to
fda3e23
Compare
Seems that I and the python-magic developer disagree on what a good public API for the magic module would be. Regarding the FACT extractor I don't think we need to copy this code since the only thing it ever needs is |
fda3e23
to
02405dd
Compare
02405dd
to
3722e15
Compare
Assuming that fkie-cad/fact_extractor#128 works and is merged this is ready for review. |
c5b6ce0
to
236a975
Compare
236a975
to
eba377d
Compare
eba377d
to
97bede4
Compare
97bede4
to
ebf366f
Compare
ebf366f
to
ab9cb48
Compare
@jstucke Assuming the CI passes, this should be mergeable as is.
Note that this is not true. Why shouldn't we drop a dependency when we can?! |
ab9cb48
to
e2022de
Compare
src/helperFunctions/magic.py
Outdated
return i | ||
|
||
|
||
def from_file(filename, magic_file=_magic_file, **kwargs) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type hints would be nice. Does it accept a str
or a Path
object? It would be nice if it accepts both
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also it does not seem to work: I get this output when calling it the first time
Warning: using regular magic file `/home/redacted/git/github/FACT_core/src/bin/firmware.mgc'
/home/redacted/git/github/FACT_core/src/bin/firmware.mgc, 1: Warning: offset `�' invalid
/home/redacted/git/github/FACT_core/src/bin/firmware.mgc, 2: Warning: offset `Firmware v' invalid
/home/redacted/git/github/FACT_core/src/bin/firmware.mgc, 3: Warning: offset `Firmware v' invalid
/home/redacted/git/github/FACT_core/src/bin/firmware.mgc, 5: Warning: offset `VENDOR' invalid
/home/redacted/git/github/FACT_core/src/bin/firmware.mgc, 20: Warning: offset `' invalid
/home/redacted/git/github/FACT_core/src/bin/firmware.mgc, 21: Warning: offset `' invalid
/home/redacted/git/github/FACT_core/src/bin/firmware.mgc, 23: Warning: offset `' invalid
/home/redacted/git/github/FACT_core/src/bin/firmware.mgc, 24: Warning: offset `' invalid
/home/redacted/git/github/FACT_core/src/bin/firmware.mgc, 25: Warning: offset `' invalid
/home/redacted/git/github/FACT_core/src/bin/firmware.mgc, 26: Warning: offset `' invalid
/home/redacted/git/github/FACT_core/src/bin/firmware.mgc, 27: Warning: offset `' invalid
/home/redacted/git/github/FACT_core/src/bin/firmware.mgc, 28: Warning: offset `' invalid
/home/redacted/git/github/FACT_core/src/bin/firmware.mgc, 31: Warning: offset `' invalid
/home/redacted/git/github/FACT_core/src/bin/firmware.mgc, 41: Warning: offset `�' invalid
/home/redacted/git/github/FACT_core/src/bin/firmware.mgc, 45: Warning: offset `�' invalid
/home/redacted/git/github/FACT_core/src/bin/firmware.mgc, 69: Warning: offset `' invalid
/home/redacted/git/github/FACT_core/src/bin/firmware.mgc, 90: Warning: offset `' invalid
/home/redacted/git/github/FACT_core/src/bin/firmware.mgc, 336: Warning: offset `
/home/redacted/git/github/FACT_core/src/bin/firmware.mgc, 342: Warning: offset `' invalid
/home/redacted/git/github/FACT_core/src/bin/firmware.mgc, 348: Warning: offset `' invalid
/home/redacted/git/github/FACT_core/src/bin/firmware.mgc, 354: Warning: offset `' invalid
/home/redacted/git/github/FACT_core/src/bin/firmware.mgc, 360: Warning: offset `' invalid
/home/redacted/git/github/FACT_core/src/bin/firmware.mgc, 365: Warning: offset `' invalid
/home/redacted/git/github/FACT_core/src/bin/firmware.mgc, 366: Warning: offset `' invalid
/home/redacted/git/github/FACT_core/src/bin/firmware.mgc, 367: Warning: offset `' invalid
/home/redacted/git/github/FACT_core/src/bin/firmware.mgc, 368: Warning: offset `' invalid
/home/redacted/git/github/FACT_core/src/bin/firmware.mgc, 377: Warning: offset `[^d]..|.[^e].|..[^b]|$)' invalid
and the mime is not correct (I get 'application/octet-stream'
even though it should be a type that is in the firmware.mgc file)
Also when trying to use the magic file directly with file
I get the same output and this:
file: File 5.45 supports only version 18 magic files. `bin/firmware.mgc' is version 14 (No such file or directory)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is actually quite interesting why it works without this changeset.
In fact_helper_file's setup.py the magic files are compiled using the system's file
.
Since the package is only ever distributed as sdist and not as a prebuilt wheel on pypi, this ensures that the
magic file version matches the version that the system's libmagic supports.
I think the way to go here is to build multiple versions of firmware.mgc to match all supported versions of libmagic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the way to go here is to build multiple versions of firmware.mgc to match all supported versions of libmagic.
Even if you target only the ones that are currently supported in FACT that would still result in five different versions:
- focal: 5.38
- jammy: 5.41
- noble: 5.45
- bullseye: 5.39
- bookworm: 5.44
Maybe compiling on the target system isn't such a bad idea. I'm liking the simplicity of the setup process of fact_helper_file
more and more where you can just install the repo as a pip package and directly use the API functions without having to deal with this at all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can just install the repo as a pip package and directly use the API functions without having to deal with this at all
While I agree the pip package is more convenient, I'd argue that the pip package abuses setup.py
for compiling.
The pip package only works when installing from git and thus compiling the magic database in the process.
The package could not be uploaded to pypi.
I'm not sure what the correct solution is here. I tend to compile the database in FACT's installer.
It is replaced by the much simpler magic.py
e2022de
to
4cb3ea5
Compare
This uses https://github.com/maringuu/fact-mime-database which is imo the only relevant thing that fact_helper_file provides.
Before merging we should somehow incoperate that fact_helper_file/mime/custom_mime_internal is missing.
If would be great if we could add this somehow add to the FACT_core repo to make fact-mime-database not use its generality.